Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to only pick assets from app/assets/stylesheets using stylesheet_link_tag :app #187

Conversation

brunoprietog
Copy link
Contributor

This is a simple proof of concept to resolve the issue raised in this comment.

The idea is that when using the helper it only includes the CSS files contained in the app/assets/stylesheets folder. This way, no external CSS styles will be included in the application, for example from engines like Mission Control.

Obviously, this could break applications that are using this and assume that they include all CSS files from all engines automatically, such as Trix. They would have to be included manually.

I see this as an intermediate option between including all CSS files from the load path or not including any and using @import manually for each file.

@dhh
Copy link
Member

dhh commented May 18, 2024

I think there's something here. But I'm thinking maybe we could let :all remain everything, thus keeping backwards compatibility, but then add :application to do what you're doing here. And then make :application the new default in new apps?

@brunoprietog
Copy link
Contributor Author

Yes, makes sense! But maybe another name? I can't think of it at the moment, but I feel like the difference between :application and "application" is very fuzzy.

@dhh
Copy link
Member

dhh commented May 18, 2024

Could do :app to match app/

@brunoprietog brunoprietog force-pushed the only-application-stylesheets-in-all-stylesheets-helper branch from 9e645e6 to 9ac2dbe Compare May 18, 2024 18:35
@brunoprietog
Copy link
Contributor Author

Sounds good. I've updated the code to cover those cases.

Let me know if there's another better way to implement it.

@@ -18,6 +18,10 @@ def load_path
@load_path ||= Propshaft::LoadPath.new(config.paths, version: config.version)
end

def app_stylesheets_load_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is the right level to have something like this. Let's keep that in the helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I've moved it to the helper. Do you think it would be better to memoize it?

@brunoprietog brunoprietog force-pushed the only-application-stylesheets-in-all-stylesheets-helper branch 2 times, most recently from a957175 to c6e766e Compare May 18, 2024 19:11
Propshaft::LoadPath.new(
[ File.join(Rails.root, "app", "assets", "stylesheets") ],
compilers: Rails.application.assets.compilers,
version: Rails.application.config.assets.version))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we can do this with Rails.application.assets.load_path.dup.tap { |l| l.paths = [ File.join(Rails.root, "app", "assets", "stylesheets") ] } or something like that. So you don't have to repeat the configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I see there's not a setter on paths. We can just add that, but it'll need to clear the cache when set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it, although I had to add a setter to clear the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I see there's not a setter on paths. We can just add that, but it'll need to clear the cache when set.

Ah yes, I just read this

@brunoprietog brunoprietog force-pushed the only-application-stylesheets-in-all-stylesheets-helper branch from c6e766e to 47ac2d6 Compare May 18, 2024 20:21
@brunoprietog brunoprietog changed the title Include only app/assets/stylesheets when using stylesheet_link_tag :all Add an option to only pick assets from app/assets/stylesheets using stylesheet_link_tag :app May 18, 2024
@brunoprietog brunoprietog force-pushed the only-application-stylesheets-in-all-stylesheets-helper branch from 47ac2d6 to 0a0e5c8 Compare May 18, 2024 20:40
def app_stylesheets_paths
stylesheets_paths_for(
Rails.application.assets.load_path.dup.tap do |load_path|
load_path.paths = [ File.join(Rails.root, "app", "assets", "stylesheets") ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to just use Rails.root.join("app/assets/stylesheets") here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on second thought, we don't need the load_path or the helper method to do this. We can just do:

def app_stylesheet_paths
  Rails.root.join("app/assets/stylesheets").then do |dir|
    dir.glob("**/*.css").collect { |f| f.relative_path_from(dir).to_s }.sort
  end
end

Although trying to think you might/probably want to cache this in production.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the cache question is an issue. Same for :all. We shouldn't be doing file stats on every request. Think we need a way to get this into a proper object with caching that's reset by a sweeper, like LoadPath.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually maybe caching doesn't matter. Just did a simple test on HEY:

haystack(dev)* Benchmark.measure { 1000.times { Rails.root.join("app/assets/stylesheets").then do |dir|
haystack(dev)*     dir.glob("**/*.css").collect { |f| f.relative_path_from(dir).to_s }.sort
haystack(dev)*     end
haystack(dev)> } }.real / 1000
=> 0.00025197141902754083

So that's an overhead of 0.2ms. Which probably means that the file system or whatever is already providing the cache.

Guess it could be worth looking at a more pathological case, maybe? Slow CPU + HDD might well give a different result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That benchmark wasn't correct. This is right:

Benchmark.measure { 1000.times { Rails.root.join("app/assets/stylesheets").then { |dir| dir.glob("**/*.scss").collect { |f| f.relative_path_from(dir).to_s } } } }.real / 1000

Which sets the price at 1ms for 263 files on a very fast CPU + SSD. That's not great. So back to looking at caching that's stable in production!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which actually gives me wonder whether we should have a generalized cache like this. Stable in production, pass-through in dev/test.

@brunoprietog brunoprietog force-pushed the only-application-stylesheets-in-all-stylesheets-helper branch from 0a0e5c8 to 8e6190a Compare May 19, 2024 01:04
@dhh
Copy link
Member

dhh commented May 19, 2024

Ended up pursuing the cached path in #190.

@dhh dhh closed this May 19, 2024
@brunoprietog
Copy link
Contributor Author

Great! Sorry for not following up on this yesterday. I had to go out

@brunoprietog brunoprietog deleted the only-application-stylesheets-in-all-stylesheets-helper branch May 20, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants